perf(mobile): compressed websocket frames for iOS - #4794
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Needs human review This PR introduces a new native iOS module for WebSocket handling with compression support. The addition of new native networking infrastructure (~400+ lines of new Swift and TypeScript code) that changes runtime WebSocket behavior warrants human review. You can customize Macroscope's approvability policy. Learn more. |
9283c97 to
4e70318
Compare
React Native's iOS WebSocket is backed by SocketRocket, which never offers permessage-deflate, so the server-side negotiation from #4705 only benefited Android (OkHttp offers it by default). This adds a URLSessionWebSocketTask- backed Expo module (URLSession offers the extension in its handshake) and injects it as the Effect Socket constructor on iOS only, with a fallback to the global WebSocket on Android and on binaries predating the module. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bots caught the emit-after-remove race: delegate callbacks deferred the emit onto the serial queue then removed the emitter synchronously, so close/error events never reached JS. Emits now run synchronously on the queue before removal. Also cleans up activeSockets when connect throws, and imports "expo" lazily so test import chains touching runtime.ts do not evaluate expo's dev-only setup (CI __DEV__ failure). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Verified on the simulator: the socket completed its 101 (with permessage-deflate negotiated) and then died instantly with NSURLErrorDomain -1005, so JS never saw an open event and the supervisor reconnected every 15s. The dev client registers custom URLProtocol classes for its network inspector; they intercept URLSession traffic and do not understand the WebSocket upgrade. Clearing protocolClasses on the session config fixes it: one stable socket, no reconnects over 60s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
URLSession retains its delegate until invalidated, so cancelAll() left the connections object and its session alive on every module destroy/recreate cycle. Teardown now invalidates and clears the session; the next open() builds a fresh one. Verified on the simulator: environment connects, holds a single socket for 60s, and reconnects cleanly after a full app teardown and relaunch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
59ce135 to
95f6b12
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 95f6b12. Configure here.
close() during CONNECTING (an Effect open-timeout or scope teardown) leaves the native connect in flight, so its didOpen could still land and flip the socket back to OPEN, firing open handlers on a socket the caller had already given up on. handleOpen now only transitions from CONNECTING. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/mobile/modules/t3-websocket/ios/T3WebSocketModule.swift (1)
30-37: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReport dropped sends instead of returning silently.
sendBinarydrops the frame when base64 decoding fails.send(line 95) drops the frame when no task exists forid. The JS adapter receives no signal in either case, so a caller sees a successfulsend()for data that never left the device. EmitonErroron both paths.♻️ Proposed change
Function("sendBinary") { (id: Int, base64: String) in - guard let data = Data(base64Encoded: base64) else { return } + guard let data = Data(base64Encoded: base64) else { + self.connections.reportError(id: id, message: "Invalid base64 payload") + return + } self.connections.send(id: id, message: .data(data)) }Add the helper and the missing-task signal in
T3WebSocketConnections:func reportError(id: Int, message: String) { queue.async { self.emitLocked(id: id, event: "onError", payload: ["message": message]) } } func send(id: Int, message: URLSessionWebSocketTask.Message) { queue.async { guard let task = self.tasksById[id] else { self.emitLocked(id: id, event: "onError", payload: ["message": "Socket is not open"]) return } // ... existing send } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/modules/t3-websocket/ios/T3WebSocketModule.swift` around lines 30 - 37, Update sendBinary and T3WebSocketConnections.send to report failed sends through onError instead of silently returning: use a T3WebSocketConnections.reportError helper for invalid base64 input, and emit an onError event with “Socket is not open” when no task exists for the requested id. Preserve the existing send behavior for valid data and open sockets.apps/mobile/src/lib/nativeWebSocket.test.ts (1)
106-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
onErrorand for protocol forwarding.The suite does not exercise
handleErroror theprotocolsargument.handleErrordispatches anErrorinstance rather than an event object, which is the least obvious part of the adapter contract. A short test for each closes that gap.💚 Suggested tests
it("dispatches native errors and forwards protocols", async () => { const construct = await loadNativeWebSocketConstructor(); const socket = construct!("wss://example.test/ws", ["t3"]); expect(lastCall("connect")!.args[2]).toEqual(["t3"]); const id = connectionIdOf(socket); const errors: unknown[] = []; socket.addEventListener("error", (event) => errors.push(event)); mocks.emit("onError", { id, message: "boom" }); expect(errors[0]).toBeInstanceOf(Error); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/src/lib/nativeWebSocket.test.ts` around lines 106 - 131, Add test coverage in the native WebSocket test suite for the constructor’s protocols argument and the onError path handled by handleError. Instantiate the socket with a protocol list and assert connect receives it, then emit onError, capture the error event, and assert the dispatched value is an Error instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mobile/src/lib/nativeWebSocket.ts`:
- Around line 46-63: Update attachNativeListeners so nativeListenersAttached is
set to true only after all four module.addListener registrations complete
successfully. Keep the flag false when any registration throws, allowing a later
invocation to retry attaching listeners and deliver events to sockets.
---
Nitpick comments:
In `@apps/mobile/modules/t3-websocket/ios/T3WebSocketModule.swift`:
- Around line 30-37: Update sendBinary and T3WebSocketConnections.send to report
failed sends through onError instead of silently returning: use a
T3WebSocketConnections.reportError helper for invalid base64 input, and emit an
onError event with “Socket is not open” when no task exists for the requested
id. Preserve the existing send behavior for valid data and open sockets.
In `@apps/mobile/src/lib/nativeWebSocket.test.ts`:
- Around line 106-131: Add test coverage in the native WebSocket test suite for
the constructor’s protocols argument and the onError path handled by
handleError. Instantiate the socket with a protocol list and assert connect
receives it, then emit onError, capture the error event, and assert the
dispatched value is an Error instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 13b4bc2a-5a6e-4d6f-9422-ebf4f5d26330
📒 Files selected for processing (6)
apps/mobile/modules/t3-websocket/expo-module.config.jsonapps/mobile/modules/t3-websocket/ios/T3WebSocket.podspecapps/mobile/modules/t3-websocket/ios/T3WebSocketModule.swiftapps/mobile/src/lib/nativeWebSocket.test.tsapps/mobile/src/lib/nativeWebSocket.tsapps/mobile/src/lib/runtime.ts
| function attachNativeListeners(module: T3WebSocketNativeModule) { | ||
| if (nativeListenersAttached) { | ||
| return; | ||
| } | ||
| nativeListenersAttached = true; | ||
| module.addListener("onOpen", (payload) => { | ||
| activeSockets.get(payload.id)?.handleOpen(); | ||
| }); | ||
| module.addListener("onMessage", (payload) => { | ||
| activeSockets.get(payload.id)?.handleMessage(payload.data ?? "", payload.binary === true); | ||
| }); | ||
| module.addListener("onClose", (payload) => { | ||
| activeSockets.get(payload.id)?.handleClose(payload.code ?? 1006, payload.reason ?? ""); | ||
| }); | ||
| module.addListener("onError", (payload) => { | ||
| activeSockets.get(payload.id)?.handleError(payload.message ?? "WebSocket error"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Set the attached flag only after the listeners attach.
If the first module.addListener call throws, nativeListenersAttached stays true. No later socket then receives native events, and every socket stays in CONNECTING until Effect's open timeout.
🛡️ Proposed change
if (nativeListenersAttached) {
return;
}
- nativeListenersAttached = true;
module.addListener("onOpen", (payload) => {
@@
module.addListener("onError", (payload) => {
activeSockets.get(payload.id)?.handleError(payload.message ?? "WebSocket error");
});
+ nativeListenersAttached = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function attachNativeListeners(module: T3WebSocketNativeModule) { | |
| if (nativeListenersAttached) { | |
| return; | |
| } | |
| nativeListenersAttached = true; | |
| module.addListener("onOpen", (payload) => { | |
| activeSockets.get(payload.id)?.handleOpen(); | |
| }); | |
| module.addListener("onMessage", (payload) => { | |
| activeSockets.get(payload.id)?.handleMessage(payload.data ?? "", payload.binary === true); | |
| }); | |
| module.addListener("onClose", (payload) => { | |
| activeSockets.get(payload.id)?.handleClose(payload.code ?? 1006, payload.reason ?? ""); | |
| }); | |
| module.addListener("onError", (payload) => { | |
| activeSockets.get(payload.id)?.handleError(payload.message ?? "WebSocket error"); | |
| }); | |
| } | |
| function attachNativeListeners(module: T3WebSocketNativeModule) { | |
| if (nativeListenersAttached) { | |
| return; | |
| } | |
| module.addListener("onOpen", (payload) => { | |
| activeSockets.get(payload.id)?.handleOpen(); | |
| }); | |
| module.addListener("onMessage", (payload) => { | |
| activeSockets.get(payload.id)?.handleMessage(payload.data ?? "", payload.binary === true); | |
| }); | |
| module.addListener("onClose", (payload) => { | |
| activeSockets.get(payload.id)?.handleClose(payload.code ?? 1006, payload.reason ?? ""); | |
| }); | |
| module.addListener("onError", (payload) => { | |
| activeSockets.get(payload.id)?.handleError(payload.message ?? "WebSocket error"); | |
| }); | |
| nativeListenersAttached = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/mobile/src/lib/nativeWebSocket.ts` around lines 46 - 63, Update
attachNativeListeners so nativeListenersAttached is set to true only after all
four module.addListener registrations complete successfully. Keep the flag false
when any registration throws, allowing a later invocation to retry attaching
listeners and deliver events to sockets.

iPhones were left out of the websocket compression we shipped in #4705. React Native's iOS WebSocket is backed by SocketRocket, which never offers permessage-deflate, so every frame to the iOS app still crossed the wire uncompressed while Android (OkHttp offers the extension by default) got the win for free.
This adds a small Apple-only Expo module backed by
URLSessionWebSocketTask, which offers permessage-deflate in its handshake automatically. A thin JS wrapper exposes the W3C WebSocket surface Effect'sSocket.fromWebSocketdrives, and the mobile runtime injects it as theSocket.WebSocketConstructoron iOS only. Android and iOS binaries predating the module fall back to the global WebSocket unchanged, so older TestFlight builds keep working.Details worth noting:
maximumMessageSizeis raised to 128 MB on the URLSession task. The default receive cap is 1 MB, and socket-fallback snapshot frames can exceed that.Tests:
vp test run src/lib/nativeWebSocket.test.ts(6 tests: id routing, once-listeners, binary round-trip, idempotent close), mobile typecheck, lint.Built with Claude Fable 5 in Claude Code.
🤖 Generated with Claude Code
Note
Medium Risk
Swaps the transport for all Effect-driven iOS WebSockets (reconnect/close semantics and large-frame handling must stay correct), but scope is limited to mobile relay sockets with explicit fallbacks and unit tests on the JS bridge.
Overview
iOS relay WebSockets now use a native
URLSessionWebSocketTaskpath so the client can negotiate permessage-deflate with the server (SocketRocket behind RN’s global WebSocket cannot). Android and older iOS builds without the module keep the existing global WebSocket.A new Apple-only Expo module (
T3WebSocket) manages connections by id, raises the receive cap to 128 MB for large snapshot frames, clears customURLProtocolclasses so dev proxies don’t break upgrades, and on transport failure emits error + synthetic close 1006 for Effect’s reconnect logic. A thin JSNativeWebSocketimplements the subset Effect needs (events,send,close, multi-connection routing), andruntime.tsinjects it asSocket.WebSocketConstructorwhenloadNativeWebSocketConstructor()succeeds.Reviewed by Cursor Bugbot for commit a7adbc1. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add compressed WebSocket native module for iOS using
URLSessionWebSocketTaskT3WebSocketModulebuilt onURLSessionWebSocketTask, supporting text/binary messaging, subprotocols, and per-connection events (onOpen,onMessage,onClose,onError) keyed by caller-supplied integer ids.NativeWebSocketclass that mirrors browser WebSocket semantics (readyState transitions, once listeners, idempotent close, base64↔Uint8Array conversion for binary frames).runtime.tsto dynamically select the native WebSocket constructor on iOS when available, falling back to the global constructor on other platforms.onClosewith code 1006.Macroscope summarized a7adbc1.
Summary by CodeRabbit
New Features
Bug Fixes
Tests